[Core] add support for reasoning parser plugins#28075
[Core] add support for reasoning parser plugins#28075DarkLight1337 merged 7 commits intovllm-project:mainfrom
Conversation
Signed-off-by: walter beller-morales <walter.beller.morales@gmail.com>
walterbm
requested review from
ProExpertProg,
WoosukKwon,
aarnphm,
benchislett,
chaunceyjiang,
hmellor,
houseroad,
mgoin,
robertgshaw2-redhat,
russellb,
simon-mo,
tlrmchlsmth,
yewentao256 and
youkaichao
as code owners
|
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run You ask your reviewers to trigger select CI tests on top of Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add If you have any questions, please reach out to us on Slack at https://slack.vllm.ai. 🚀 |
mergify
bot
added
deepseek
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for dynamically loading reasoning parser plugins, following a similar pattern to tool parser plugins. The changes introduce a new --reasoning-parser-plugin flag and the necessary logic to handle plugin loading and parser validation.
My review identifies two main issues. First, the custom argparse.Action for the reasoning parser is dependent on the order of command-line arguments, which can lead to unexpected failures. I've suggested a more robust implementation. Second, there's redundant code in the StructuredOutputManager, including a duplicate variable assignment and unnecessary plugin loading, which I've recommended simplifying. Addressing these points will improve the robustness and maintainability of the new feature.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Signed-off-by: walter beller-morales <walter.beller.morales@gmail.com>
chaunceyjiang
added
the
ready
|
/cc @aarnphm PTAL, |
aarnphm
left a comment
aarnphm
left a comment
There was a problem hiding this comment.
discussed on slack, i will take this into account when doing refactoring.
Signed-off-by: walter beller-morales <walter.beller.morales@gmail.com>
Signed-off-by: walter beller-morales <walter.beller.morales@gmail.com>
Signed-off-by: walter beller-morales <walter.beller.morales@gmail.com>
|
This pull request has merge conflicts that must be resolved before it can be |
Signed-off-by: walter beller-morales <walter.beller.morales@gmail.com>
| "--reasoning-parser", | ||
| # This choice is a special case because it's not static | ||
| choices=list(ReasoningParserManager.list_registered()), | ||
| # Choices need to be validated after parsing to include plugins |
There was a problem hiding this comment.
It might have been nicer if choices were changed to metavar so that users using the CLI --help can still see the built in options without the passed value being strictly checked. You could add something like <plugin> so that users know the list in the metavar is not exhaustive.
@walterbm could this be done in a follow up?
There was a problem hiding this comment.
@hmellor good idea! yes I can do that
Signed-off-by: walter beller-morales <walter.beller.morales@gmail.com>
Signed-off-by: walter beller-morales <walter.beller.morales@gmail.com>
6 participants
Purpose
Allow reasoning parsers to be dynamically loaded through a new
--reasoning-parser-pluginflag (following the pattern used by tool parsers & tool parser plugins). This will make it easier for newer other reasoning models (like Cohere models) to experiment and implement reasoning parsers.Test Plan
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.